feat(query-orchestrator): evaluate interval refresh keys from local time - #11614

Todos

• Gather context / read diff
• Review BaseQuery descriptor emission
• Review query-orchestrator consumption
• Review env flag, tests, docs
• Post review

What I verified

The core refactor holds up. everyRefreshKeyParts() is a faithful extraction: the non-cron branch reproduces utcOffset/parseSecondDuration exactly, the cron branch delegates unchanged to calcIntervalForCronString, and everyRefreshKeySql now renders from those same values — so the SQL genuinely cannot drift from the descriptor. everyRefreshKeySql has no overrides anywhere in src/, and the Tesseract planner does not generate refresh keys (PreAggregations.ts:320 routes through BaseQuery.preAggregationInvalidateKeyQueries), so there is no second code path this misses.

The query parameter threaded into localRefreshKeyOptions is the right call — with externalQueryClass, everyRefreshKeySql delegates to the external instance, and the descriptor has to come from that same instance. The MssqlQuery test covers it.

Opt-out coverage is correct at all four emit sites: refreshKey.sql (cube and pre-agg) returns before the descriptor is attached, and incremental is excluded because the key gets wrapped in CASE WHEN NOW() < <dateTo> against an allocated partition-range param. The conservative choice to opt out even when the timeDimensions guard skips the wrapping is right.

The floor-arithmetic claim in evaluateLocalRefreshKey's comment checks out: for integer x, f ∈ [0,1), interval ≥ 1, the half-open range (x, x+f] contains no integer, so no multiple of interval lies in it and the fractional milliseconds can never move the bucket.

isValidLocalRefreshKey is a genuinely useful guard, not defensive noise — calcIntervalForCronString produces utcOffset: NaN when this.timezone is set but refreshKey.timezone is not (pre-existing, BaseQuery.js:4826-4828), and this catches it and falls back rather than emitting a garbage key.

I also confirmed @AsyncDebounce on loadRefreshKey only dedups in-flight calls (the LRU entry is deleted in finally), so it cannot pin a stale local value; and renewQuery reads the refresh keys exactly once per call, so the split-brain risk the PreAggregationLoadCache memo guards against does not have an analogue on the QueryCache side.

Findings

Smaller observations (not worth inline threads)

• QueryCacheOptions.localRefreshKey is reachable from QueryOrchestrator (...options.queryCacheOptions) but the Joi schema at optionsValidate.ts:127 uses .keys({...}) without the field, so user config would be rejected — it is effectively test-only. That's fine, but it means the emit side (env, via server.ts → CompilerApi) and the consume side (env, via getEnv fallback) are two independent reads of the same variable. Both default off and both read the same env, so no realistic split, but a comment noting they must stay in lockstep would help.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally, so a failure leaks 'yes' into later tests. Matches the surrounding file's style, so take it or leave it.
• base-query.test.ts 'emits no descriptor when the flag is off' will fail if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set in the environment. Consider passing localRefreshKey: false explicitly.
• No test covers a cron-based key end-to-end through PreAggregationLoadCache with the flag on — the pieces are each tested, the seam is not.

Praise

The everyRefreshKeyParts agrees with the SQL it renders test — evaluating the emitted FLOOR(...) string as JS with a pinned clock and comparing against the descriptor arithmetic — is exactly the right property to assert for a refactor whose whole safety argument is "these two cannot disagree." Gating at emit time so the existing everyRefreshKeySql assertions pass unedited is also the correct proof that the SQL didn't move, and the boundary-stability test on the per-request memo shows the split-brain hazard was thought through rather than stumbled into.

Security: nothing to flag. The descriptor is compiler-produced numbers, validated before use, and never reaches SQL.

@@             Coverage Diff             @@
##           master   #11614       +/-   ##
===========================================
- Coverage   79.91%   58.87%   -21.04%     
===========================================
  Files         498      237      -261     
  Lines      104694    18982    -85712     
  Branches     3841     3850        +9     
===========================================
- Hits        83661    11175    -72486     
+ Misses      20472     7246    -13226     
  Partials      561      561               

Flags with carried forward coverage won't be shown. Click here to find out more.

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Todos

• Read the rebased diff
• Re-check prior findings against the new consumption site (cacheRefreshKeyResult)
• Review new code (memoization key fix, utils, tests)
• Resolve stale threads / post updated review

What changed since the last review

The branch was rebased onto master with #11607 (refreshKeyIdentity consolidation) in it, and the consumption side was reworked accordingly. Three things are materially different:

1. The short-circuit moved from two sites to one. QueryCache.loadRefreshKey and PreAggregationLoadCache.keyQueryResult are no longer touched; the gate now sits at the top of QueryCache.cacheRefreshKeyResult (QueryCache.ts:523), which both funnel through. I verified via call-site grep that these really are the only two entries — PreAggregationLoader.ts:398,407 and PreAggregationPartitionRangeLoader.ts:121 all go through keyQueryResult, and loadRefreshKeysFromQuery / renewQuery go through loadRefreshKeys. Strictly better than the previous version; one owner instead of two parallel copies.
2. The PreAggregationLoadCache TODO is gone, because fix(query-orchestrator): key refresh key cache entries consistently #11607 fixed the underlying key mismatch upstream (PreAggregationLoadCache.ts:212 now calls refreshKeyCacheKey, matching what line 194 stores under). Thread resolved.
3. A real bug was found and fixed: the compiler-level queryCache memo key at BaseQuery.js:290 now includes localRefreshKey, next to convertTzForRawTimeDimension. Without it a flag-on query's compiled refresh keys would leak to a flag-off query in the same process. Good catch, and correctly placed.

Re-verification of the parts that didn't change

The everyRefreshKeyParts extraction still holds: the non-cron branch reproduces utcOffset/parseSecondDuration exactly, the cron branch delegates unchanged, and everyRefreshKeySql renders from those same values. query || this in localRefreshKeyOptions is still the right instance under externalQueryClass. Opt-out coverage is intact at all four emit sites — refreshKey.sql returns before the descriptor is attached in both refreshKeysByCubes (BaseQuery.js:4255) and preAggregationInvalidateKeyQueries (BaseQuery.js:5051), and incremental is excluded explicitly at BaseQuery.js:5088.

Return shape matches the SQL path ([{ refresh_key: n }]), so renewQuery's refreshKeyValues and PreAggregationLoader.contentVersion see the same structure. buildRangeInvalidateKey derives from the tuple, not from a cache lookup, so nothing breaks by the value never being persisted.

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Open findings

Findings 2–5 are unchanged in the current diff, so I did not re-post them; the existing unresolved threads still carry them. Finding 1's old thread went outdated when the line moved, so I resolved it and re-anchored the comment at cacheRefreshKeyResult, noting the renewQuery amplification path that the rebase makes more visible.

On finding 1 specifically: the commit message now documents the one-time rebuild and the clock-skew blast radius thoroughly, but the refreshKeyRenewalThreshold interaction is the one operational surprise not covered anywhere — commit message, docs, or code.

Smaller observations

• QueryCacheOptions.localRefreshKey is reachable from QueryOrchestrator via ...options.queryCacheOptions, but optionsValidate.ts (packages/cubejs-server-core/src/core/optionsValidate.ts:127-129) uses .keys({...}) without the field, so user config would be rejected — it stays test-only. Fine, but it means emit (env → server.ts → CompilerApi) and consume (env → getEnv fallback) are two independent reads of the same variable. Both default off and read the same env, so no realistic split; a one-line comment noting they must stay in lockstep would help the next reader.
• base-query.test.ts:1830 'emits no descriptor when the flag is off' will fail if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set in the ambient environment, since the test constructs PostgresQuery without localRefreshKey and BaseQuery.js:367 falls back to getEnv. Passing localRefreshKey: false explicitly would pin it.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally, so a failure leaks 'yes' into later tests. Matches the surrounding file's style.
• Still no end-to-end test of a cron-based key through PreAggregationLoadCache with the flag on — the pieces are each tested, the seam is not. everyRefreshKeyParts returning {...calcIntervalForCronString(refreshKey), cron: true} is the branch where a non-zero dayOffset and a NaN utcOffset are both possible, so it is the branch most worth an integration assertion.

Praise

The everyRefreshKeyParts agrees with the SQL it renders test — evaluating the emitted FLOOR(...) string as JS with a pinned clock and comparing against the descriptor arithmetic — remains exactly the right property for a refactor whose whole safety argument is "these two cannot disagree." The rebase onto #11607 genuinely improved the design rather than just resolving conflicts: collapsing to a single short-circuit at cacheRefreshKeyResult removed the duplicated gate. And the keyQueryResult is stable across an interval boundary within one load cache test, which stubs Date.now across a boundary and asserts the memo pins the value, is the test that proves the split-brain hazard was designed around rather than stumbled past.

Todos

• Read the current diff
• Re-check prior findings against current code
• Review newly changed code (env-only gate, threshold opt-out, RefreshScheduler)
• Resolve stale threads / post review

What changed since the last review

Three material changes, all in response to the earlier round:

1. The prior medium is fixed. QueryCache.localRefreshKeyResult (QueryCache.ts:252) now returns null and logs a warning when options.refreshKeyRenewalThreshold is set, so the local path can no longer silently restore an invalidation cadence a deployment deliberately throttled. The comment explaining why a cached value bounds key advancement is accurate, and runs the query when refreshKeyRenewalThreshold is configured (QueryCache.abstract.ts) pins it. I resolved that thread. Note the per-key queryOptions.renewalThreshold is correctly not consulted — refreshKeyRenewalThresholdForInterval returns min(interval/10, 300), always at or below the interval, so it never throttles.
2. The localRefreshKey QueryCacheOptions field is gone; the gate is read straight from getEnv('refreshKeyLocalTime') in the constructor, with a comment explaining that an independently-settable option would make the feature a no-op. This closes my earlier "two independent reads must stay in lockstep" observation — there is now one read on each side of the same variable and no way to set them apart. Tests were reworked to toggle the env var around construction, which is the honest way to test it.
3. New: RefreshScheduler.refreshCubesRefreshKey skips warming (RefreshScheduler.ts:346-362) for cubes without a sql refresh key when the flag is on. The reasoning is right — there is no shared cache entry to warm, so the getSql + executeQuery per timezone would be thrown away. 'sql' in cubeFromPath.refreshKey is also correctly more conservative than BaseQuery.refreshKeysByCubes' truthiness check (BaseQuery.js:4256), so it errs toward warming. This is where the new medium lands, below.

Re-verification of the unchanged parts

everyRefreshKeyParts still reproduces the old everyRefreshKeySql branches exactly (utcOffset/parseSecondDuration inline, cron delegated unchanged), and the SQL renders from those same values, so descriptor and SQL cannot drift. query || this in localRefreshKeyOptions is still the right instance under externalQueryClass, covered by the MssqlQuery tests. Opt-out coverage is intact at all four emit sites — refreshKey.sql returns before the descriptor is attached in both refreshKeysByCubes and preAggregationInvalidateKeyQueries, and incremental is excluded explicitly. PreAggregationLoadCache.keyQueryResult still memoizes under refreshKeyCacheKey and wraps cacheRefreshKeyResult, so the local value is pinned for the life of one load — the boundary-stability test proves it. Return shape [{ refresh_key: n }] matches the SQL path.

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

I could not run the test suites in this job (the sandbox declined the npx jest invocation), so the assertions above are from reading the code, not from a green run.

Findings

Findings 3–5 are unchanged in the current diff, so I did not re-post them; the existing unresolved threads carry them. I also left the logLocalRefreshKeyOnce comment nit unresolved — the comment still claims subclass field initialization makes this.logger unusable, which isn't so (logger is a parameter property, assigned before the constructor body), and the real justification is that the warning should fire only when a descriptor is genuinely used. GitHub marks that thread outdated because the comment moved onto the new helper; the concern is unchanged.

Smaller observations

• base-query.test.ts 'emits no descriptor when the flag is off' constructs PostgresQuery without localRefreshKey, and BaseQuery.js:367 falls back to getEnv, so the test fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. Passing localRefreshKey: false explicitly would pin it. The orchestrator tests were fixed for exactly this class of leak in this push; the compiler test still has it.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally, so a failure leaks 'yes' into later tests. Matches the surrounding file's style.
• logLocalRefreshKeyOnce shares one boolean across both messages, so whichever fires first suppresses the other forever. Unreachable in practice — refreshKeyRenewalThreshold is static per QueryCache — but it makes the flag name (localRefreshKeyLogged) a little optimistic.
• The compiler-level memo key at BaseQuery.js:290 uses this.options.localRefreshKey (possibly undefined) rather than the resolved this.localRefreshKey. Two CompilerApis in one process that resolve to the same effective value via different routes would key differently — a wasted cache slot, never a wrong result, so not worth changing.
• Still no end-to-end test of a cron-based key through PreAggregationLoadCache with the flag on. That is the branch where a non-zero dayOffset and a NaN utcOffset are both reachable, so it is the one most worth an integration assertion.

Praise

The everyRefreshKeyParts agrees with the SQL it renders test — evaluating the emitted FLOOR(...) string as JS against a pinned clock and comparing to the descriptor arithmetic — remains exactly the right property for a refactor whose whole safety argument is "these two cannot disagree." Dropping the QueryCacheOptions.localRefreshKey field in favour of a single env read is the better call even though it made the tests uglier: it removes a way to configure the feature into a silent no-op, and the tests paid the honest price rather than keeping a test-only seam around. And the keyQueryResult is stable across an interval boundary within one load cache test is the one that shows the split-brain hazard was designed around rather than stumbled past.

Todos

• Read the new commit (5e9d484) in full
• Verify it is docs/comment-only, no behavioural change
• Re-check prior findings against current code
• Resolve stale threads / post review

What changed since the last review

One commit, 5e9d484 "docs: rewrite CUBEJS_REFRESH_KEY_LOCAL_TIME in plainer language". Two files, 21 insertions / 18 deletions, and no executable lines: the environment-variables.mdx section was rewritten and the refreshKeyLocalTime docblock was dropped from env.ts. I re-read QueryCache.localRefreshKeyResult, cacheRefreshKeyResult, utils.ts and the RefreshScheduler hunk to confirm nothing moved — they're byte-identical to what I reviewed at c93362d, so the whole safety argument from that round carries over unchanged.

The rewrite itself is a real improvement. Leading with what Cube does today ("Cube normally asks the database what time it is") before what the flag changes is the right order, and the clock-sync warning now explains why the requirement exists ("trusting the machine's clock instead of a single shared source of time") rather than just stating it. The new [ref-config-query-cache-options] link target resolves — config.mdx:510 is ### \orchestrator_options`, so #orchestrator_options` is a valid anchor.

Dropping the env.ts docblock is fine: getEnv entries around it are mixed on this, and the clock-skew caveat now lives in both the docs and the runtime warning at QueryCache.ts:263.

One thing the docs change makes newly visible: config.mdx's own orchestrator_options example sets refreshKeyRenewalThreshold: 30. Anyone who copied that example verbatim now has this flag silently disabled — which is exactly the "presence, not throttling" shape of the earlier low finding, and an argument for quantizing rather than opting out. Not a defect in this diff; noting it because the docs now cross-link the two pages.

I did not run the suites in this job; the assertions above are from reading the diff. Given the push is docs-only, the previously reported results still stand.

Findings

Two posted on the rewritten section, both low:

Carried over, unchanged in this diff, existing threads still hold them:

Resolved two of my own threads: the refreshKeyRenewalThreshold docs ask at QueryCache.ts:252 (the alternative I offered was "quantize, or say so in the docs" — this push says so), and the outdated docs thread, re-anchored as finding B above.

Smaller observations (unchanged, not worth threads)

• base-query.test.ts 'emits no descriptor when the flag is off' constructs PostgresQuery without localRefreshKey and BaseQuery.js:367 falls back to getEnv, so it fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. The orchestrator tests were hardened against exactly this leak; the compiler test still has it.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally, so a failure leaks 'yes' into later tests. Matches the surrounding file's style.
• Still no end-to-end test of a cron-based key through PreAggregationLoadCache with the flag on — the branch where a non-zero dayOffset and a NaN utcOffset are both reachable, so the one most worth an integration assertion.

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Praise

Rewriting the env var section to explain the mechanism before the switch, and to name the refreshKeyRenewalThreshold interaction outright, is the version an operator can actually act on — the earlier draft described the implementation, this one describes the decision. The everyRefreshKeyParts agrees with the SQL it renders test and the keyQueryResult is stable across an interval boundary within one load cache test remain the two assertions carrying this PR's safety argument, and both are untouched.

Todos

• Read the new commit (8956c9a) in full
• Verify it is docs-only, no behavioural change
• Re-check prior findings against current code
• Resolve stale threads / post review

What changed since the last review

One commit, 8956c9a "docs: tighten CUBEJS_REFRESH_KEY_LOCAL_TIME". One file, 13 insertions / 20 deletions, zero executable lines — git diff --stat 5e9d484..HEAD shows only environment-variables.mdx. So the whole safety argument from the c93362d round carries over untouched: everyRefreshKeyParts / everyRefreshKeySql, QueryCache.localRefreshKeyResult, cacheRefreshKeyResult, utils.ts and the RefreshScheduler hunk are byte-identical to what I reviewed then.

The tightening is good editing. The section went from ~19 lines of prose to ~11 and lost nothing load-bearing:

• The opening dropped the SELECT FLOOR(...) mechanism walkthrough in favour of "Cube asks the database what time it is. If true, it uses its own clock instead" — the mechanism was interesting to a reviewer, not to an operator deciding whether to flip a switch.
• The refreshKeyRenewalThreshold carve-out is now stronger, not just shorter: "So do all refresh keys if you have set queryCacheOptions.refreshKeyRenewalThreshold" states the scope (every key, not some) that the previous "also stay on the database path" phrasing left ambiguous. This is the clearest that interaction has been stated in any draft.
• The warning kept both halves of the causal chain — clocks must agree, and the consequence is the same pre-aggregation built twice — in two sentences instead of four.

One thing dropped that I'd have kept: the old text said "including cron expressions". A reader with every: '0 5 * * *' now has to infer that a cron string counts as "every-based". True (BaseQuery.everyRefreshKeyParts routes cron through calcIntervalForCronString under the same every key), and the inference isn't hard — noting it rather than posting a thread.

I did not run the suites in this job; the assertions above are from reading the diff. Given the push is docs-only, the previously reported results still stand.

Findings

No new defects. Both docs findings from the previous round survive the rewrite verbatim in substance, so I resolved the two outdated threads and re-anchored them on the current lines:

Carried over, unchanged in this diff, existing threads still hold them:

Smaller observations (unchanged, not worth threads)

• config.mdx's own orchestrator_options example sets refreshKeyRenewalThreshold: 30. Anyone who copied that example verbatim now has this flag silently disabled — the docs are at least explicit about it as of this push, but it's an argument for quantizing rather than opting out.
• base-query.test.ts 'emits no descriptor when the flag is off' constructs PostgresQuery without localRefreshKey and BaseQuery.js:367 falls back to getEnv, so it fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. The orchestrator tests were hardened against exactly this leak; the compiler test still has it.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally, so a failure leaks 'yes' into later tests. Matches the surrounding file's style.
• Still no end-to-end test of a cron-based key through PreAggregationLoadCache with the flag on — the branch where a non-zero dayOffset and a NaN utcOffset are both reachable, so the one most worth an integration assertion.

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Praise

Three docs passes in and this one is the version that earns its space: shorter than the last draft and clearer about the one thing an operator can get wrong, because "so do all refresh keys if you have set refreshKeyRenewalThreshold" names the scope instead of gesturing at it. Cutting the SELECT FLOOR(...) explanation was the right thing to cut — it was there for reviewers, not readers. The everyRefreshKeyParts agrees with the SQL it renders test and the keyQueryResult is stable across an interval boundary within one load cache test remain the two assertions carrying this PR's safety argument, and both are untouched.

Todos

• Read the new commit (d715c4b) in full
• Verify it is comment-only, no behavioural change
• Re-check prior findings against current code
• Resolve stale threads / dedupe against existing ones

What changed since the last review

One commit, d715c4b "refactor: drop the DST note from LocalRefreshKeyDescriptor". git diff --stat 8956c9a..HEAD is a single file, 4 deletions, 0 insertions, zero executable lines — the LocalRefreshKeyDescriptor docblock in QueryCache.ts lost its trailing paragraph:

utcOffset is frozen at compile time, exactly as it already is when baked into the emitted SQL string — the orchestrator must never recompute it, or the local and SQL paths would stop agreeing across a DST transition.

Everything else — everyRefreshKeyParts / everyRefreshKeySql, localRefreshKeyResult, cacheRefreshKeyResult, utils.ts, the RefreshScheduler hunk, all tests — is byte-identical to what I reviewed at c93362d, so the whole safety argument from that round carries over untouched.

One note on the trim itself, since this is the one comment removal in the series I'd have kept. Unlike the c93362d trims, this paragraph wasn't restating adjacent code — it was the only place recording an invariant that the code cannot express: that utcOffset must be consumed as given and never recomputed on the orchestrator side. Nothing violates it today (evaluateLocalRefreshKey reads descriptor.utcOffset straight through, utils.ts:45), so there is no defect here; the note was forward-looking guidance for whoever next touches utils.ts, and the DST failure mode it names is not obvious from the surrounding four fields. The remaining first paragraph still gives the formula, which is the more important half. Noting rather than posting a thread — comment content at this granularity is the author's call, and the invariant is at least still implied by the descriptor being compiler-produced.

I did not run the suites in this job; the assertions above are from reading the diff. Given the push is comment-only, the previously reported results still stand.

Findings

No new defects. All open threads still apply to the current diff, so per dedupe I did not re-post any of them — the unresolved state carries them:

Nothing to resolve this round: every unresolved thread of mine is still applicable, and the already-resolved ones stayed resolved. Finding 6's comment moved by 4 lines but the thread is still anchored on it, so no re-anchor needed either.

Smaller observations (unchanged, not worth threads)

• config.mdx's own orchestrator_options example sets refreshKeyRenewalThreshold: 30. Anyone who copied it verbatim now has this flag silently disabled — the docs are explicit about it as of 8956c9a, but it remains an argument for quantizing rather than opting out.
• base-query.test.ts 'emits no descriptor when the flag is off' constructs PostgresQuery without localRefreshKey and BaseQuery.js:367 falls back to getEnv, so it fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. The orchestrator tests were hardened against exactly this leak; the compiler test still has it.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally, so a failure leaks 'yes' into later tests. Matches the surrounding file's style.
• Still no end-to-end test of a cron-based key through PreAggregationLoadCache with the flag on — the branch where a non-zero dayOffset and a NaN utcOffset are both reachable.

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Praise

Five pushes in, the diff has only gotten easier to review — this one is four lines of prose, which costs a reviewer nothing to confirm. The everyRefreshKeyParts agrees with the SQL it renders test and the keyQueryResult is stable across an interval boundary within one load cache test remain the two assertions carrying this PR's safety argument, and both are untouched.

Todos

• Read the three new commits (2131ec4, 4f5f6d2, 151b09b)
• Re-check prior findings against current code
• Review the new RefreshScheduler gate and the string-return change
• Resolve stale threads / post review

What changed since the last review

Three commits, and two of them are real behaviour changes that close prior findings.

1. 2131ec4 — the medium is fixed, and fixed the way I'd hoped. QueryCache now exposes isLocalRefreshKeyActive() (QueryCache.ts:243) as the single predicate, localRefreshKeyResult consumes it, and RefreshScheduler.refreshCubesRefreshKey asks the orchestrator for it (RefreshScheduler.ts:346-350) instead of re-reading getEnv. So a deployment with refreshKeyRenewalThreshold set keeps its scheduled warming, which is exactly the case that was regressing. keeps warming interval keys when refreshKeyRenewalThreshold vetoes local evaluation pins it end-to-end through a real RefreshScheduler and a mock driver, and reports whether local evaluation is in effect pins the predicate itself with the comment explaining why it must agree with the branches above it. Thread resolved.

Two details I checked while verifying: hoisting getOrchestratorApi(context) out of the timezone loop is safe (it's memoized per context, and scheduled refresh already creates it), and the skip is still sound because the executeQuery it guards passes loadRefreshKeysOnly: true — there is genuinely nothing else being warmed by that call.

One residual: isLocalRefreshKeyActive() can't see the per-key rejection in isValidLocalRefreshKey, so a cube whose descriptor is malformed (the utcOffset: NaN a cron every produces when refreshKey.timezone is unset) would still lose warming while running as a query. I'm not asking for anything here — if that state is reachable, the emitted SQL is already FLOOR((NaN + …)) and broken independently of this PR, so it's the wrong thing to design the warming gate around.

2. 4f5f6d2 — returning a string is the right call, and better justified than I expected. RefreshScheduler.ts:60 already declares refreshKeyValues: {refresh_key: string}[][], so the codebase's own type was on the string side before this PR. I checked every consumer of these values: they are only ever hashed (contentVersion, the query-cache renewalKey) or compared for equality, never used arithmetically — grep for refresh_key across both packages' src/ turns up nothing else. So the change is safe and it removes the one-time full pre-aggregation rebuild for the common deployment, since an every key runs against Cube Store whenever externalQueryClass is set (BaseQuery.js:4896) and Cube Store carries every column as a string. The docblock states this reasoning accurately. I narrowed my old docs thread rather than dropping it: with no external store the key hits the source DB, and FLOOR in BigQuery is FLOAT64 → a JS number, so the toggle still moves contentVersion once there.

3. 151b09b — the docs correction is the honest version. "saves a round trip on nearly every request" was overstated, since each result is cached for a fraction of the interval; "one per refresh key per renewal window, multiplied by every cube, tenant, and timezone" is what actually happens, and it's still a good reason to want the flag. Naming that scheduled refresh stops issuing the warming queries too is a nice touch — it makes the 2131ec4 behaviour user-visible in the place someone would look.

Also in this push: the Local refresh key evaluation enabled log line is gone, so the clock-sync caveat now lives only in the docs, and logLocalRefreshKeyOnce has exactly one caller — which retires the "one shared flag suppresses both messages" half of that nit.

I could not run the suites in this job (the sandbox declined both npx jest and node -e), so everything above is from reading the code. The author's previously reported results still stand for the unchanged parts; the new RefreshScheduler and string-shape tests are unverified by me.

Findings

No new defects. Status of the threads:

Findings 2, 4 and 5 are unchanged in this diff, so I did not re-post them; the unresolved threads carry them. Resolved three of my own: the RefreshScheduler medium (fixed), the old docs rebuild thread (its central claim — "returns a JS number" — is now false), and the old outdated flake thread (re-anchored).

Smaller observations (not worth threads)

• base-query.test.ts 'emits no descriptor when the flag is off' constructs PostgresQuery without localRefreshKey and BaseQuery.js:367 falls back to getEnv, so it fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. The new RefreshScheduler tests do the right thing here (delete process.env.… in beforeEach); the compiler test still has the leak.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally.
• Every cron case in base-query.test.ts passes refreshKey.timezone explicitly, so the utcOffset: NaN branch that isValidLocalRefreshKey exists to catch has no test. Still the branch most worth an integration assertion, now with a second reason: it's also the branch where the RefreshScheduler gate and the QueryCache gate can disagree.

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Praise

isLocalRefreshKeyActive() is the small right answer to that finding — one predicate, named, tested, and consulted from both sides, rather than a second copy of the condition in the scheduler. And 4f5f6d2 is the kind of fix that only comes from taking a review note seriously and then digging past it: the string return isn't just "avoid the rebuild", it's a demonstration that the SQL path's own shape was a string all along, right down to the type annotation already sitting in RefreshScheduler. The everyRefreshKeyParts agrees with the SQL it renders and keyQueryResult is stable across an interval boundary tests still carry the safety argument, and both survived intact.

Todos

• Read the new commit (a9672ca) in full
• Re-check prior findings against current code
• Resolve stale threads / post review

What changed since the last review

One commit, a9672ca "refactor(query-orchestrator): make the local refresh key flag an option" — three files, and it partly reverses the earlier env-only decision:

1. QueryCacheOptions.localRefreshKey is back, and the constructor resolves options.localRefreshKey ?? getEnv('refreshKeyLocalTime') (QueryCache.ts:231). The comment explaining why the option was removed is gone with it.
2. All logging on this path is gone — logLocalRefreshKeyOnce, the localRefreshKeyLogged flag and both messages. I grepped: no leftover references. In its place is a TODO on the threshold opt-out proposing to snap the local value to the threshold rather than fall back to a query, which is the fix I'd suggested; recording it and moving on is a reasonable call. This retires the logLocalRefreshKeyOnce comment nit outright, so I resolved that thread.
3. Both test helpers drop the env-toggling wrapper in favour of passing the option. PreAggregations.test.ts does this correctly (localRefreshKey, in the options object). QueryCache.abstract.ts does not — see below.

Findings

Findings 3–6 are unchanged in this diff, so I did not re-post them; the unresolved threads carry them. Resolved one thread (the logLocalRefreshKeyOnce comment nit, now moot).

On finding 1 specifically: this is the second time in this PR that a test-seam change quietly disabled the thing under test, and the tests are the entire safety argument here. Worth running yarn unit in packages/cubejs-query-orchestrator before merge — I could not run it in this job (the sandbox declined every npx jest invocation), so the failure prediction above is from reading the code, not from a red run. The reasoning is mechanical, though: the parameter is never referenced inside the constructed options object, and cacheOptions only ever carries refreshKeyRenewalThreshold.

Re-verification of the unchanged parts

everyRefreshKeyParts / everyRefreshKeySql, localRefreshKeyResult's decline branches, utils.ts, PreAggregationLoadCache.keyQueryResult's memo and the RefreshScheduler gate are otherwise byte-identical to 151b09b. isLocalRefreshKeyActive() is still the single predicate consulted by both localRefreshKeyResult and RefreshScheduler.refreshCubesRefreshKey (RefreshScheduler.ts:346-350), so the fix from 2131ec4 holds for the env-driven configuration. Return shape is still [{ refresh_key: string }].

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Smaller observations (not worth threads)

• The new does not report the declined local evaluation when … cases assert the absence of a log line that no code emits any more. Harmless as a regression guard, but note it currently passes trivially — and, per finding 1, for two of the three cases the flag isn't even set.
• base-query.test.ts 'emits no descriptor when the flag is off' still falls back to getEnv, so it fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. Now that the orchestrator side has a real option again, the compiler test could pin localRefreshKey: false the same way.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally.
• Still no end-to-end test of a cron key through PreAggregationLoadCache with the flag on — the branch where a non-zero dayOffset and a NaN utcOffset are both reachable.

Praise

The TODO on the threshold opt-out is the honest way to close that discussion: the conservative behaviour ships, the better behaviour is named in the one place someone would look. And splitting localRefreshKeyResult out into direct unit tests — valid, missing, malformed, threshold-vetoed, string-typed — is a better shape than only asserting through loadRefreshKeys; it will be genuinely useful once the helper actually applies the flag.

Todos

• Read the new commit (13d8373)
• Re-check prior findings, especially the newCache high
• Resolve stale threads / post review

What changed since the last review

One commit, 13d8373 "refactor: drop comments that restate the local refresh key code" — four files, 13 insertions / 23 deletions. Three of the four hunks are comment-only, and the fourth is the fix for the high finding:

The high is fixed. QueryCache.abstract.ts:486 now spreads the parameter through — { ...options, localRefreshKey, ...cacheOptions } — instead of hardcoding localRefreshKey: undefined. So the flag-on cases in the local refresh key block actually run with the flag on again. I re-read the whole block against the current QueryCache:

• evaluates locally without touching the driver (514) → localRefreshKeyEnabled true, valid descriptor, no threshold → localRefreshKeyResult returns before queryWithRetryAndRelease, so executed === 0 holds.
• reports whether local evaluation is in effect (571) → enabled true / disabled false / throttled false all match isLocalRefreshKeyActive() (QueryCache.ts:239).
• localRefreshKeyResult → evaluates a valid descriptor (632) and returns the key as a string (639) get a value rather than null, and the non-null assertion on the destructure is now sound.
• The four negative cases still pass, and now for the right reason.

QueryCacheOptions is already imported at the top of the file and carries localRefreshKey?: boolean (QueryCache.ts:174), so the Partial<QueryCacheOptions> signature type-checks. Thread resolved.

I could not run the suite in this job (the sandbox declined yarn unit), so the above is from reading the code — but it's the mechanical inverse of the failure I predicted last round, so please confirm with one green run before merge.

The comment trims are fine. The reworded evaluateLocalRefreshKey docblock keeps both load-bearing halves (why fractional milliseconds can't move the bucket; why the return is a string) in fewer words, and the String(...) justification is still the strongest sentence in the file. Two cosmetic notes, neither worth a thread: everyRefreshKeyParts and localRefreshKeyOptions lost @param {Object} refreshKey while keeping their other tags, so the JSDoc is now asymmetric (harmless — BaseQuery.js isn't // @ts-checked).

Findings

No new defects. Open threads, all unchanged in this diff, so not re-posted:

Resolved one thread this round: the newCache high, now genuinely fixed.

Re-verification of the unchanged parts

everyRefreshKeyParts / everyRefreshKeySql, localRefreshKeyResult's decline branches, isValidLocalRefreshKey, PreAggregationLoadCache.keyQueryResult's memo and the RefreshScheduler gate (RefreshScheduler.ts:347,365, consulting isLocalRefreshKeyActive() through the orchestrator) are byte-identical to a9672ca. Return shape is still [{ refresh_key: string }].

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Smaller observations (unchanged, not worth threads)

• The does not report the declined local evaluation when … cases assert the absence of a log line no code emits, so they pass trivially. Harmless as a regression guard now that the flag is genuinely applied.
• base-query.test.ts 'emits no descriptor when the flag is off' still falls back to getEnv, so it fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. Now that the orchestrator tests pin the option explicitly, the compiler test could do the same with localRefreshKey: false.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally.
• Still no end-to-end test of a cron key through PreAggregationLoadCache with the flag on — the branch where a non-zero dayOffset and a NaN utcOffset are both reachable.

Praise

Turning a one-line test-seam bug into a smaller, cleaner helper — the comment moved above the object literal and the spread does the work — is the right size of fix. And the trimmed evaluateLocalRefreshKey docblock is a good example of shortening without losing the argument: the String(...) rationale survived intact, and that's the sentence a future reader will need.

Todos

• Read the new commit (01043d1)
• Verify the test-helper change actually keeps the flag-on cases on the local path
• Re-check prior findings against current code
• Resolve stale threads / dedupe against existing ones

What changed since the last review

One commit, 01043d1 "fix(query-orchestrator): take the local refresh key flag from the cache test options" — one file, 40 insertions / 49 deletions, all in test/unit/QueryCache.abstract.ts. No src/ file moved, so the whole safety argument from the earlier rounds carries over untouched.

It replaces the two-argument shape (localRefreshKey?: boolean + cacheOptions) with a single additionalOptions: Partial<QueryCacheOptions> spread over the shared options, and every call site now says { localRefreshKey: true } (or false, or nothing) in that one bag. I traced each case against QueryCache as it stands:

• newCache({ localRefreshKey: true }) → { ...options, localRefreshKey: true } → localRefreshKeyEnabled = options.localRefreshKey ?? getEnv(...) = true (QueryCache.ts:231). The flag genuinely reaches the constructor now.
• evaluates locally without touching the driver → valid descriptor, no threshold → localRefreshKeyResult returns before queryWithRetryAndRelease, so executed === 0 holds.
• reports whether local evaluation is in effect → true / false / false matches isLocalRefreshKeyActive() (QueryCache.ts:239).
• localRefreshKeyResult → evaluates a valid descriptor and returns the key as a string get a value, so the ! on the destructure is sound.
• The negative cases (flag is off, flag is unset, no descriptor, malformed descriptor, refreshKeyRenewalThreshold configured) all still take the SQL path, each for its intended reason. flag is unset now carries the comment that used to sit in newCache, which is the right place for it — that case is the only one where the getEnv fallback is the thing under test.

This is a better shape than 13d8373's spread-the-parameter fix: with one bag there is no second place for the flag to come from, so the shadowing that caused the earlier high can't reappear. Type-wise it also gets a small guard for free — additionalOptions is Partial<QueryCacheOptions>, so putting a descriptor there instead of a boolean is a compile error, while queryOptions stays any. The reverse mistake (a boolean in queryOptions) still type-checks and would silently land on the SQL path via isValidLocalRefreshKey, but it would fail the assertion loudly, so it isn't a trap.

I could not run the suite in this job — there is no node_modules in the checkout (neither root nor package) and installing wasn't available here, so the above is from reading the code rather than a green run. Since this commit is exactly the mechanical inverse of the failure I predicted at a9672ca, one yarn unit in packages/cubejs-query-orchestrator before merge would close it out.

Findings

No new defects. Every open thread of mine still applies to the current diff, so per dedupe I did not re-post any — the unresolved state carries them:

Nothing to resolve this round: all five are still applicable, and the previously resolved threads (including the newCache high) stayed resolved and correctly so.

Re-verification of the unchanged parts

everyRefreshKeyParts / everyRefreshKeySql, localRefreshKeyResult's decline branches, isLocalRefreshKeyActive(), isValidLocalRefreshKey, evaluateLocalRefreshKey, PreAggregationLoadCache.keyQueryResult's memo and the RefreshScheduler gate (RefreshScheduler.ts:347,365, consulting isLocalRefreshKeyActive() through the orchestrator) are byte-identical to 13d8373. Return shape is still [{ refresh_key: string }].

Security: nothing to flag. The descriptor is compiler-produced numbers, validated by isValidLocalRefreshKey before use, and never reaches SQL.

Smaller observations (unchanged, not worth threads)

• The does not report the declined local evaluation when … cases assert the absence of a log line no code emits, so they pass trivially — harmless as a regression guard.
• base-query.test.ts 'emits no descriptor when the flag is off' still constructs PostgresQuery without localRefreshKey and BaseQuery.js:367 falls back to getEnv, so it fails if CUBEJS_REFRESH_KEY_LOCAL_TIME=true is set ambiently. The orchestrator tests now pin the option explicitly in one consistent place; the compiler test could do the same with localRefreshKey: false.
• env.test.ts refreshKeyLocalTime(exception) deletes the env var after the expect rather than in a finally.
• Still no end-to-end test of a cron key through PreAggregationLoadCache with the flag on — the branch where a non-zero dayOffset and a NaN utcOffset are both reachable.

Praise

The right follow-up to the 13d8373 fix, and a strictly smaller API: collapsing two ways of supplying the flag into one removes the class of bug rather than the instance, and the comment about the unset case moved onto the single test that actually exercises it. Nine pushes in, the diff keeps getting cheaper to review.